Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add globalSearch x-pack plugin #66293

Merged
merged 55 commits into from
Jun 4, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented May 12, 2020

Summary

First stage of #61657
Implementation of #64284

  • Implement GS RFC as the globalSearch x-pack plugin

Checklist

@pgayvallet pgayvallet added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label May 12, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet pgayvallet added the REASSIGN from Team:Core UI Deprecated label for old Core UI team label May 13, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core-ui (Team:Core UI)

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels May 13, 2020

import { of, throwError } from 'rxjs';
import supertest from 'supertest';
import { UnwrapPromise } from '@kbn/utility-types';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a side note: Finally we will get rid of this microsoft/TypeScript#27711

const factory = getContextFactory(coreStart);
const context = factory(request);

expect(coreStart.savedObjects.getScopedClient).toHaveBeenCalledTimes(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's worth adding such a detailed test. The only logic in the factory that uiSettings.client uses the same soClient instance. Which can be considered an implementation detail.

@@ -9,6 +9,7 @@ import { RESERVED_DIR_JEST_INTEGRATION_TESTS } from '../../../src/dev/constants'
export default {
rootDir: '../../',
roots: [
'<rootDir>/plugins',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this file used? x-pack/scripts/jest uses another config file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, but only by the integration test config. The jest command in xpack does not, and use the file you linked instead 🙈

import config from './config';
export default {
...config,

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The last pending question is #66293 (comment)


```ts
startDeps.globalSearch.find('some term').subscribe(
({ results }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: while this form is completely fine in the code when IDE gives you hints, it's not reader-friendly when used in the docs. What if we provide a verbose example:

startDeps.globalSearch.find('some term').subscribe({
  next ({ results }) {
    addNewResultsToList(results);
  },
  error(){},
  complete() {
    showAsyncSearchIndicator(false);
  }
});

});
```

## Known limitations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can link the RFC for more details provided

): string => {
if (typeof url === 'string') {
// relative path
if (url.indexOf('/') === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: url.startsWith('/') is a bit clearer

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit c5546f4 into elastic:master Jun 4, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jun 4, 2020
* add skeleton for global_search plugin

* base implementation of the server-side service

* add utils tests

* add server-side mocks

* move take_in_array to common folder

* implements base of client-side plugin

* add tests for server-side service

* fix server plugin tests

* implement `navigateToUrl` core API

* extract processResults for the client-side

* fetch server results from the client side

* factorize process_results

* fix plugin start params

* move things around

* move all server types to single file

* fix types imports

* add basic FTR tests

* add client-side service tests

* add tests for addNavigate

* add getDefaultPreference & tests

* use optional for RequestHandlerContext

* add registerRoutes test

* add base test for context

* resolve TODO

* common nits/doc

* common nits/doc on public

* update CODEOWNERS

* add import for declare statement

* add license check on the server-side

* add license check on the client-side

* eslint

* address some review comments

* use properly typed errors for obs

* add integration tests for the find endpoint

* fix unit tests

* use licensing start contract

* translate the error message

* fix eslint rule for test_utils

* fix test_utils imports

* remove NavigableGlobalSearchResult, use `application.navigateToUrl` instead.

* use coreProvider plugin in FTR tests

* nits

* fix service start params

* fix service start params, bis

* I really need to fix this typecheck oom error

* add README, update missing jsdoc

* nits on doc
# Conflicts:
#	.github/CODEOWNERS
#	rfcs/text/0011_global_search.md
pgayvallet added a commit that referenced this pull request Jun 4, 2020
* add skeleton for global_search plugin

* base implementation of the server-side service

* add utils tests

* add server-side mocks

* move take_in_array to common folder

* implements base of client-side plugin

* add tests for server-side service

* fix server plugin tests

* implement `navigateToUrl` core API

* extract processResults for the client-side

* fetch server results from the client side

* factorize process_results

* fix plugin start params

* move things around

* move all server types to single file

* fix types imports

* add basic FTR tests

* add client-side service tests

* add tests for addNavigate

* add getDefaultPreference & tests

* use optional for RequestHandlerContext

* add registerRoutes test

* add base test for context

* resolve TODO

* common nits/doc

* common nits/doc on public

* update CODEOWNERS

* add import for declare statement

* add license check on the server-side

* add license check on the client-side

* eslint

* address some review comments

* use properly typed errors for obs

* add integration tests for the find endpoint

* fix unit tests

* use licensing start contract

* translate the error message

* fix eslint rule for test_utils

* fix test_utils imports

* remove NavigableGlobalSearchResult, use `application.navigateToUrl` instead.

* use coreProvider plugin in FTR tests

* nits

* fix service start params

* fix service start params, bis

* I really need to fix this typecheck oom error

* add README, update missing jsdoc

* nits on doc
# Conflicts:
#	.github/CODEOWNERS
#	rfcs/text/0011_global_search.md
@joshdover joshdover mentioned this pull request Jul 6, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REASSIGN from Team:Core UI Deprecated label for old Core UI team release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants